Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use consolidate to make template engine configurable #236

Merged
merged 5 commits into from
Feb 6, 2014

Conversation

fyockm
Copy link
Contributor

@fyockm fyockm commented Jan 17, 2014

Step 1 in making the template engine configurable, use consolidate.js. I don't really like how the file extensions are named now, with some as .html and others as .jade. Need to think on that topic a little more. Might make sense to update the directory structure to split the file types.

@lirantal could be good to give swig a try instead of jade, as that might help with the extension problem.

@lirantal
Copy link
Contributor

yeah I think consolidate sounds good as a wrapper for templating and I'd definitely go for swig as the default.
some templating is required in the server side as well so you can generate at least the skeleton HTML document and then ng-app style directives can take it from there with client side templating.

@fyockm
Copy link
Contributor Author

fyockm commented Jan 22, 2014

@lirantal do you want to do the swig migration? or should I give it a go?

@liorkesos
Copy link
Member

Hi guys,
Before we jump in - my 2 cents..
I like the consolidate approach and we'll assess it.
I think we should spend more time on choosing the default template.
My guidelines are that it should be fast and should have syntax as similar as possible to angular's for variable inclusion - hence {{}} notation.
Please see these links..

  1. http://garann.github.io/template-chooser/
  2. http://paularmstrong.github.io/node-templates/benchmarks.html (which reinforces swig).

@fyockm
Copy link
Contributor Author

fyockm commented Jan 22, 2014

@liorkesos The top 2 template engines on from Paul's list swig and whiskers aren't even options on Garann's. So, that makes it difficult to compare between them. I also question the currency of the benchmarks, as it's been 2 years since that site was update.
I agree that the brackets notation would be preferable.
With all that said, I don't have a better suggestion than swig at this point.

@lirantal
Copy link
Contributor

@fyockm I'm all up for Swig, you're welcome to go ahead and update your pull request with a change for using it as the default.
@liorkesos I've posted the same benchmarks earlier on our Facebook page when rooting for Swig, and it was the only up-to-date benchmarking comparison I could find :)

Guys, I've been working with both Swig and AngularJS for a short while now and the only comment I have so far is that Swig uses the same curly braces {{ }} notation which means that if you create a server side template which angular is supposed to use and you intend the curly braces to be used by angular then that won't work because Swig, on the server side, will evaluate the curly braces first.

What I did on the projects I'm working on is to use Swig's {% raw %} handler so that it doesn't attempt to evaluate the block that's inside it. It's not the cleanest way to go, probably the better approach is to simply include angular templates instead of writing them directly in the server templates. Another option is to change angular's curly braces notation from {{ }} to [[ ]] which is definitely an easy way out but I fear its somewhat annoying and confusing.

@fyockm
Copy link
Contributor Author

fyockm commented Feb 3, 2014

@lirantal @liorkesos
Thanks in part to @samof76, who's work I found on issue #219. He had already done some of the swig work, though he didn't submit a Pull Request for some reason. semantic-ui was also implemented in his version, so I had to retrofit bootstrap.

@liorkesos
Copy link
Member

I'm currently getting messages complaining of missing mongoose and warnings about ..

Local Npm module "grunt-mocha-test" not found. Is it installed?
I'll dive deeper - cause something is astray..

@fyockm
Copy link
Contributor Author

fyockm commented Feb 4, 2014

@liorkesos npm cache clean - Similar to rebooting a Windows machine :-)

@fyockm fyockm mentioned this pull request Feb 6, 2014
liorkesos added a commit that referenced this pull request Feb 6, 2014
use consolidate to make template engine configurable
@liorkesos liorkesos merged commit f0eb648 into linnovate:master Feb 6, 2014
@fyockm
Copy link
Contributor Author

fyockm commented Feb 6, 2014

@liorkesos Thanks!

@liorkesos
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants